Skip to content

Move quoted.{Expr|Type}.apply to scala.internal #6046

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Mar 12, 2019
Merged

Move quoted.{Expr|Type}.apply to scala.internal #6046

merged 2 commits into from
Mar 12, 2019

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Mar 8, 2019 •
edited
Loading

This is to avoid having this internal representation leaked to the user with scala.quoted.Expr.apply. Users could have accidentally quoted a term using Expr(myTerm) which does not get the correct level in the context while typing.

@nicolasstucki nicolasstucki self-assigned this Mar 8, 2019
@nicolasstucki nicolasstucki force-pushed the remove-compiletime-methods-from-expr-an-type branch 2 times, most recently from 04e9e21 to b241c0b Compare March 8, 2019 15:37
@nicolasstucki nicolasstucki marked this pull request as ready for review March 8, 2019 17:28
@nicolasstucki nicolasstucki requested a review from liufengyun March 8, 2019 17:28
liufengyun
liufengyun reviewed Mar 10, 2019
View reviewed changes
Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

BTW, what is the consideration to favor a separate package instead of a different method name that could avoid accidental usage as well?

@nicolasstucki
Copy link
Contributor Author

If we only changed the method name but kept it in scala.quoted.Expr it would be exposed when completing Expr. in the IDE or REPL.

@nicolasstucki
Copy link
Contributor Author

Actually, the compiletime package is not the best place to put it.

@nicolasstucki nicolasstucki force-pushed the remove-compiletime-methods-from-expr-an-type branch from c921547 to 95090fd Compare March 10, 2019 18:08
@nicolasstucki nicolasstucki changed the title Move quoted.{Expr|Type}.apply to compiletime.quoted.{Expr|Type}.apply Move quoted.{Expr|Type}.apply to quoted.internal.Compiletime Mar 10, 2019
@nicolasstucki
Copy link
Contributor Author

@liufengyun I moved thing around

@odersky
Copy link
Contributor

odersky commented Mar 10, 2019

Let's discuss the name.

We have scala.compiletime as user-facing the inline support package, and scala.internal for things that only the compiler should know about. So I think this should in a package object in scala.internal. If we must use Compiletime it should be in lower case.

@nicolasstucki
Copy link
Contributor Author

Then it should be in scala.internal.Expr or scala.internal.quoted.Expr

@nicolasstucki nicolasstucki changed the title Move quoted.{Expr|Type}.apply to quoted.internal.Compiletime Move quoted.{Expr|Type}.apply to scala.internal Mar 11, 2019
@nicolasstucki nicolasstucki force-pushed the remove-compiletime-methods-from-expr-an-type branch from 95090fd to caf3f11 Compare March 11, 2019 12:55
@nicolasstucki nicolasstucki mentioned this pull request Mar 11, 2019
Merged
@nicolasstucki
Copy link
Contributor Author

@liufengyun now the logic was moved to scala.internal which should finally be the correct place to put it in.

liufengyun
liufengyun approved these changes Mar 12, 2019
View reviewed changes
Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nicolasstucki nicolasstucki merged commit fcfab9c into scala:master Mar 12, 2019
@ghost ghost removed the stat:needs review label Mar 12, 2019
@nicolasstucki nicolasstucki deleted the remove-compiletime-methods-from-expr-an-type branch March 12, 2019 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liufengyun liufengyun liufengyun approved these changes

Assignees

@liufengyun liufengyun

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nicolasstucki @odersky @liufengyun